runtime: select BuildKit explicitly + fix tar symlink encoding#47
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the generated Dockerfile ChangesDocker BuildKit and symlink handling
Sequence Diagram(s)sequenceDiagram
participant PrePull as prePullBaseImages
participant Tar as tarDirectory
participant DockerClient as ImageBuild
participant Stream as streamBuildOutput
PrePull->>DockerClient: pull resolved base images (best-effort)
Tar->>DockerClient: stream build context tar (symlinks preserved)
DockerClient->>Stream: emit build JSON-line records (BuildKit/classic)
Stream->>Client: forward parsed output lines
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ImageBuild was called without setting ImageBuildOptions.Version, so dockerd routed every build through its classic builder. Classic synthesizes one intermediate container per Dockerfile step and routes every container API through the daemon's authorization pipeline. Behind an authz plugin (DAP's dap-authz, any equivalent on consumer hosts) the per-step round-trip storm turns a sub-second build into a multi-minute one — observed ~140× slowdown on a 7-step Dockerfile against a 7.7GB base. BuildKit uses a single streaming session and is unaffected. Set Version: build.BuilderBuildKit. The moby /build endpoint accepts a tar request body in BuildKit mode (verified empirically), so no session / filesync wiring is required for our usage. Per-step progress events are dropped under BuildKit because the aux trace records are protobuf-encoded and decoding them would require pulling in github.com/moby/buildkit; BuildStart / BuildCompleted still fire and errors still propagate. Also fix a long-standing bug in tarDirectory: symlinks were emitted as TypeSymlink with an empty Linkname (the call to tar.FileInfoHeader passed "" instead of os.Readlink(path)). Some tar readers reject those as malformed and abort the build mid-stream — common in compose-primary contexts containing node_modules/.bin/* or similar bin-symlinks. Now readlink the target and pass it through. Drop the `# syntax=docker/dockerfile:1.4` directive from the features Dockerfile: classic ignored it, but BuildKit treats it as a hard frontend-pull requirement that needs a session for credential forwarding (which we don't open) and hangs behind broken registry mirrors. The emitted Dockerfile uses no 1.4-only syntax. Same directive in useruid.go is addressed in PR fix/uid-reconcile-syntax-directive. Integration coverage: test/integration/build_context_symlink_test.go builds a workspace whose context contains an unreferenced symlink and asserts the build completes — regression guard for the tar encoding bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
59e248d to
7873300
Compare
BuildKit refuses remote image-metadata resolution without an active
session ("no active sessions"), even for fully anonymous public-image
pulls — the session is the mandatory callback channel for registry
credentials. The previous commit set Version: BuilderBuildKit and
worked locally only because images were already in dockerd's local
store; CI (cold cache) hit the session error.
Side-step the session entirely by pre-pulling each FROM image via
the classic ImagePull API (which doesn't require a session). Once
the image is in the local store, BuildKit picks up the cached
manifest and skips remote resolution.
Implementation:
- extractBaseImages does a naive Dockerfile parse: literal FROMs,
$VAR / ${VAR} substitution against ARG defaults and BuildSpec
Args, --platform=... and AS <stage> stripping, stage-name
detection to avoid pulling intermediate stages.
- prePullBaseImages calls ImageInspect first (skip if local) then
PullImage best-effort (silently ignore errors so locally-built
images like dc-go-base-* / dc-go-final-* don't fail when there's
no remote to pull from; if the image really can't be obtained,
the subsequent ImageBuild surfaces a clearer error).
Avoids pulling in github.com/moby/buildkit as a direct dep (~100+
transitive packages). The parser is intentionally minimal and drops
FROMs with unresolved vars rather than guessing — a missed pre-pull
just falls back to BuildKit's native error path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@runtime/docker/build_test.go`:
- Around line 8-14: The TestExtractBaseImages test struct has inconsistent field
alignment causing gofmt to fail; open the struct in TestExtractBaseImages and
reformat the fields (name, dockerfile, buildArgs, want) to have consistent
spacing/indentation, then run gofmt -w runtime/docker/build_test.go (or run your
editor's gofmt) to apply the canonical formatting so CI passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51eb8f8e-cb43-4c1d-b036-07011f5104e1
📒 Files selected for processing (2)
runtime/docker/build.goruntime/docker/build_test.go
Summary
runtime/docker/build.gowas callingImageBuildwithout settingImageBuildOptions.Version, so dockerd routed every build through its classic builder. Classic synthesizes one intermediate container per Dockerfile step; each container API call is authz-checked. Behind an authz plugin this turns a sub-second build into a multi-minute one (~140× slowdown observed on a 7-step Dockerfile against a 7.7GB base — productionuid_reconcilebuilds were taking ~200s where BuildKit takes ~1.5s on the same host).Version: build.BuilderBuildKitis sufficient — the moby/buildendpoint accepts a tar request body in BuildKit mode, so nosession/filesyncwiring is required (verified empirically against a real dockerd before writing the patch). This avoids pulling ingithub.com/moby/buildkitas a direct dep.BuildStart/BuildCompletedstill fire and errors still propagate; per-vertex progress is deferred to a future PR if needed.tarDirectory: symlinks were emitted asTypeSymlinkwith an emptyLinkname, which some tar readers reject as malformed. Common in compose contexts containingnode_modules/.bin/*or similar bin-symlinks. Nowos.Readlink(path)is passed totar.FileInfoHeader.# syntax=docker/dockerfile:1.4directive fromfeature/dockerfile.go: classic ignored it, but BuildKit treats it as a hard frontend-pull requirement that needs a session for credential forwarding (we don't open one) and hangs behind broken registry mirrors. Emitted Dockerfile uses no 1.4-only syntax. The identical fix inuseruid.gois on branchfix/uid-reconcile-syntax-directive.BuildKit is now a hard requirement. Docker Engine has shipped with BuildKit enabled by default since 23.0 (Feb 2023); this is in line with the lib's modern-spec stance.
Test plan
go test ./...)TestBuildContext_SurvivesSymlinksbuilds a workspace whose context contains an unreferenced symlink — regression guard for the tar encoding buggo test -tags integration ./test/integration/, ~52s)feature/dockerfile_test.goVersion=BuilderBuildKit+ tar request body builds successfully without a session🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests